-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[ROCDL] Added missing named barrier ops (gfx1250) #162488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: None (ravil-mobile) ChangesThis patch introduces some missing s.barrier instructions in the ROCDL dialect handling named barriers Specifically: Tests:
cc @krzysz00, @amd-eochoalo Full diff: https://github.com/llvm/llvm-project/pull/162488.diff 3 Files Affected:
|
5ea786d to
5135c4f
Compare
| @@ -292,18 +292,56 @@ def ROCDL_BarrierOp : ROCDL_Op<"barrier"> { | |||
| let assemblyFormat = "attr-dict"; | |||
| } | |||
|
|
|||
| def ROCDLBufferLDS : LLVM_PointerInAddressSpace<3>; | |||
|
|
|||
| def ROCDL_BarrierInitOp : ROCDL_IntrOp<"s.barrier.init", [], [], [Pure], 0, 0, 0, 0, [1], ["id"]>, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this pure? I don't see any results returned by this op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think this is pure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... Heck, the LLVM has IntrHasSideEffects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah should not be pure. this op does not have a return value so it will get eliminated if it is pure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Removed the trait. As I can see there is not HasSideEffect trait in MLIR. So, I left the list of traits empty
kuhar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add descriptions saying which gfxip version introduced these? Example: https://github.com/ravil-mobile/llvm-project/blob/5135c4fdae71ef27192ec190298f3a00ba85ae64/mlir/include/mlir/Dialect/LLVMIR/ROCDLOps.td#L1043-L1045
| } | ||
|
|
||
| def ROCDL_BarrierJoinOp : ROCDL_IntrOp<"s.barrier.join", [], [], [Pure], 0>, | ||
| Arguments<(ins Arg<ROCDLBufferLDS, "", [MemRead]>:$ptr)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to mark this as MemRed - the LLVM intrinsics are marked NoMem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. You are right. All this functions are marked with IntrNoMem trait in LLVM. Removed it
|
All LLVM intrinsics added in this op have property |
| def ROCDLBufferLDS : LLVM_PointerInAddressSpace<3>; | ||
|
|
||
| def ROCDL_BarrierInitOp : ROCDL_IntrOp<"s.barrier.init", [], [], [Pure], 0, 0, 0, 0, [1], ["id"]>, | ||
| Arguments<(ins Arg<ROCDLBufferLDS, "", [MemRead]>:$ptr, I32Attr:$id)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AMDGPU/s-barrier.ll#L83
so the s_barrier_init takes a single m0 so it should not read mem.
Same is signal.var, join.
|
The discrepancy here being: barriers need to be defined as a global variable at the LLVM IR level. |
5135c4f to
41c84bc
Compare
Done |
|
just want to make sure: do you know if on the rocdl level, is there any way we can declare a |
krzysz00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm
|
@lialan To answer your question, the declarations I see in tests for these named barriers are So, at the MLIR level, you just need a (No, I don't know what the So future work will be adding |
@krzysz00 Yeah I do not see the wrapper for type |
|
@lialan You don't need such a wrapper, just use (We can add a wrapper to AMDGPU, but we don't need one in |
This patch introduces some missing s.barrier instructions in the ROCDL dialect handling named barriers Specifically: ``` @llvm.amdgcn.s.barrier.init - s_barrier_init @llvm.amdgcn.s.barrier.join - s_barrier_join @llvm.amdgcn.s.barrier.leave - s_barrier_leave @llvm.amdgcn.s.barrier.signal.isfirst - s_barrier_signal_isfirst @llvm.amdgcn.s.get.barrier.state - s_get_barrier_state ```
This patch introduces some missing s.barrier instructions in the ROCDL dialect handling named barriers Specifically: ``` @llvm.amdgcn.s.barrier.init - s_barrier_init @llvm.amdgcn.s.barrier.join - s_barrier_join @llvm.amdgcn.s.barrier.leave - s_barrier_leave @llvm.amdgcn.s.barrier.signal.isfirst - s_barrier_signal_isfirst @llvm.amdgcn.s.get.barrier.state - s_get_barrier_state ```
This patch introduces some missing s.barrier instructions in the ROCDL dialect handling named barriers
Specifically:
Tests:
cc @krzysz00, @amd-eochoalo